Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Refactor and unify {render, input}-order #918

Merged
merged 3 commits into from
Oct 22, 2024
Merged

Conversation

Drakulix
Copy link
Member

Draft because this needs more testing.

In theory this should fix:

Essentially this tries to use common code for iterating through all of the surfaces/clients/whatever that can be rendered and receive input.

Unfortunately this means dropping SplitRenderElements, as that was/is a source of disagreement between the input-code (which doesn't have a similar method) and the render-code.

As such this duplicates a bit of code, but usually the popup code path is much shorter anyway, so we introduce separate code paths for popups and toplevels all over.

This also cleans up some of the code introduced as part of the focus-follows-cursor (and vise versa) mode, which duplicated some more of the input logic. Everything should now either use State::surface_under or State::element_under depending if it would affect pointer or keyboard focus.

Initial tests seem promising and don't show any obvious breakage, however this definitely needs more testing and possibly some in-depth reviews, though I would ideally like to merge this before it starts to seriously bitrot.

@Drakulix Drakulix requested a review from ids1024 October 10, 2024 21:30
@git-f0x
Copy link
Contributor

git-f0x commented Oct 10, 2024

Testing this with floating windows, it seems I can't click on anything outside a rectangle in the top-left corner slightly larger than a quarter of the screen (edit: panels are clickable). Though the size of the clickable area seems to be variable (I think it changes when maximizing and unmaximizing an app?).
Tiling and maximized windows seem to behave as expected.
Clicking on Workspaces in Settings in the image below will focus the Firefox window behind (also I couldn't click the attach button in GitHub, regardless of window position, even though it was highlighted on hover).
screenshot-2024-10-10-22-57-46

Edit: But I can confirm that this also fixes #418. 🙃

@Drakulix
Copy link
Member Author

Thanks for testing! Last commit should fix the bugginess around floating windows.

@git-f0x
Copy link
Contributor

git-f0x commented Oct 11, 2024

The unclickable area issue seems to be fixed! Though there still seem to be some focus issues, where clicking on a window in the background doesn't focus it and bring it to the front (though they are interactable), and input sometimes goes through to the window below.
screenshot-2024-10-11-16-03-23
Clicking on the cosmic files or Nautilus window here doesn't focus them and bring them to the front.
While trying to attach this screenshot, clicking on the Open button in the cosmic-files dialog just focused (and brought to front) the (maximized) Firefox window below, until the dialog was moved towards the top-left screen corner.

Edit: Input seems to only go through when there's a maximized window below, and it goes through only outside the similarly sized top-left anchored rectangle as above. With tiling, clicks in a floating window in front sometimes visually move focus to the window behind.

@Drakulix
Copy link
Member Author

Yeah, stupid of me, the element_under code for floating windows (related to keyboard focus) needed the exact same fixes as surface_under (related to pointer focus) before.

@git-f0x
Copy link
Contributor

git-f0x commented Oct 11, 2024

I'm not encountering any issues from some initial testing with the latest changes. :)
Edit: Testing with a game, it seems the cursor is offset from it's actual position, and can't be moved all the way to the top-left corner (this doesn't happen with current master). Though it might be game-specific (I only have one installed for testing on COSMIC).
But this could also be an interaction between the weird window behavior on game startup and this PR (and so the fix might be out-of-scope for this PR).

@ids1024
Copy link
Member

ids1024 commented Oct 11, 2024

I guess render_input_order should just return an iterator, but that's maybe awkward until rust has a generator syntax with yield. Seems like a good way to do things for now.

@Drakulix
Copy link
Member Author

@git-f0x Good observation, the new code failed to take fullscreen surfaces correctly into account.

@Drakulix Drakulix marked this pull request as ready for review October 21, 2024 16:10
@Drakulix
Copy link
Member Author

Seems to work fine after a day of extensive testing / running in production. Lets merge

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants